Skip to content

[Files] Add user ID to file#144026

Merged
jloleysens merged 31 commits into
elastic:mainfrom
jloleysens:files-record-created-by
Nov 7, 2022
Merged

[Files] Add user ID to file#144026
jloleysens merged 31 commits into
elastic:mainfrom
jloleysens:files-record-created-by

Conversation

@jloleysens
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens commented Oct 26, 2022

Screenshot

  • Adds the user flattened field to the base files metadata. This contains id and name.

Screenshot 2022-11-01 at 11 23 44

Blocked by: #144272

Checklist

@jloleysens jloleysens added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed Team:AppServicesUx Feature:Files v8.6.0 labels Oct 26, 2022
@jloleysens jloleysens requested a review from a team as a code owner October 26, 2022 12:07
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

@jloleysens jloleysens changed the title [Files] Record created by field [Files] Record created_by field Oct 26, 2022
@jloleysens jloleysens changed the title [Files] Record created_by field [Files] Add user ID to file Oct 31, 2022
@jloleysens jloleysens requested a review from a team as a code owner October 31, 2022 10:45
alt,
meta,
mime: mimeType,
owner: security?.authc.getCurrentUser(req)?.username,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure that just username is enough to identify the user, as I saw this in the search code:

/**
* The realm type/name & username uniquely identifies the user who created this search session
*/
realmType?: string;
realmName?: string;
username?: string;

I'd ask @elastic/kibana-security for a review if we haven't already

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point @Dosant . We reached out to security team and they advised us to use profile_uid. Changes incoming.

Comment thread x-pack/plugins/files/common/types.ts Outdated
* for searching and filtering.
*/
Meta?: Meta;
Meta?: Meta & FileSystemMetadata;
Copy link
Copy Markdown
Contributor Author

@jloleysens jloleysens Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to the Meta field because I was running into the 1000 fields upper limit for the SO index

Log link

Happy to use the regular declaration for new fields, but since we already have a flattened field that kind of fits this use case, I pivoted the implementation to use that instead. cc @vadimkibana

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also notified core about this limit being hit in our functional tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A there any downsides of storing it under meta? Are these still searchable/aggregatable / filterable?

@jloleysens jloleysens removed the request for review from a team October 31, 2022 11:23
@jloleysens jloleysens enabled auto-merge (squash) October 31, 2022 11:29
@jloleysens jloleysens disabled auto-merge October 31, 2022 11:29
@jloleysens jloleysens enabled auto-merge (squash) October 31, 2022 11:30
@jloleysens
Copy link
Copy Markdown
Contributor Author

A there any downsides of storing it under meta? Are these still searchable/aggregatable / filterable?

Here is a list of the downsides: https://www.elastic.co/guide/en/elasticsearch/reference/master/flattened.html#supported-operations

The fields are essentially the same as "keyword" fields which for usernames and user IDs should work quite well.

@jloleysens jloleysens disabled auto-merge October 31, 2022 13:03
@vadimkibana
Copy link
Copy Markdown
Contributor

vadimkibana commented Oct 31, 2022

The problems of putting those fields in meta are:

  1. Then those fields don't follow ECS anymore.
  2. meta should be left for our customers to use to store their custom metadata, we should use something else to store our data.
    • Those fields should be read-only, our consumers should not be able to edit user profile ID and username, by editing the meta field.

For profile ID, ideally we would use file.owner or file.user.id, which both are per ECS, AFAIK.

* main:
  Upgrade @elastic/makelogs from v6.0.0 to v6.1.1 (elastic#144231)
  [Files] move to src (elastic#144044)
  [Synthetics UI] Add pagination and date filtering to test runs table (elastic#144029)
  Update time range when opening timeline from Entity Analytics page (elastic#144024)
  [Security Solution] Added guided onboarding for the rules area (elastic#144016)
@Dosant Dosant self-requested a review October 31, 2022 15:52
jloleysens and others added 13 commits November 1, 2022 11:07
* main: (43 commits)
  [Synthetics] Step details page screenshot (elastic#143452)
  [Lens] Datatable expression types improvement. (elastic#144173)
  [packages/kbn-journeys] start apm after browser start and stop after browser is closed (elastic#144267)
  [Files] Make files namespace agnostic (elastic#144019)
  Implement base browser-side logging system (elastic#144107)
  Correct wrong multiplier for byte conversion (elastic#143751)
  [Monaco] Add JSON syntax support to the Monaco editor (elastic#143739)
  CCS Smoke Test for Remote Clusters and Index Management  (elastic#142423)
  [api-docs] Daily api_docs build (elastic#144294)
  chore(NA): include progress on Bazel tasks (elastic#144275)
  [RAM] Allow users to see event logs from all spaces they have access to (elastic#140449)
  [APM] Show recommended minimum size when going below 5 minutes (elastic#144170)
  [typecheck] delete temporary target_types dirs in packages (elastic#144271)
  [Security Solution][Endpoint] adds new alert loading utility and un-skip FTR test for endpoint (elastic#144133)
  [performance/journeys] revert data_stress_test_lens.ts journey step (elastic#144261)
  [TIP] Use search strategies in Threat Intelligence (elastic#143267)
  Optimize react-query dependencies (elastic#144206)
  [babel/node] invalidate cache when synth pkg map is updated (elastic#144258)
  [APM] AWS lambda estimated cost (elastic#143986)
  [Maps] layer group wizard (elastic#144129)
  ...
Copy link
Copy Markdown
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jloleysens jloleysens enabled auto-merge (squash) November 4, 2022 10:16
Copy link
Copy Markdown
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, looks great and is consistent with ECS / naming convention in audit log!

@jloleysens jloleysens merged commit 1428251 into elastic:main Nov 7, 2022
@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id before after diff
file 10 11 +1
Unknown metric groups

API count

id before after diff
files 276 278 +2

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 58 64 +6
osquery 108 113 +5
securitySolution 440 446 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 66 72 +6
osquery 109 115 +6
securitySolution 517 523 +6
total +20

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Nov 7, 2022
@jloleysens jloleysens deleted the files-record-created-by branch November 7, 2022 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This PR does not require backporting Feature:Files release_note:skip Skip the PR/issue when compiling release notes v8.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants